Skip to content

feat(detected): replace suggestions with detected books#57

Open
serabi wants to merge 14 commits intodevfrom
fix-suggestion-revamp
Open

feat(detected): replace suggestions with detected books#57
serabi wants to merge 14 commits intodevfrom
fix-suggestion-revamp

Conversation

@serabi
Copy link
Copy Markdown
Owner

@serabi serabi commented Apr 6, 2026

Summary

  • replace the old suggestions flow with detected books on the dashboard, including ABS and KoSync started items before they are fully added
  • remove the legacy suggestions UI, API endpoints, and frontend assets while keeping upgrade-safe migrations and adding an Alembic merge migration for existing installs
  • disable the legacy rescan path by default, move cache cleanup to DetectedBook, remove the final ABS PendingSuggestion path, and pin mistune to 3.2.0 so the dev image builds again

Testing

  • python3 -m py_compile src/services/suggestion_service.py src/sync_manager.py src/services/kosync_service.py
  • python3 -m py_compile src/blueprints/dashboard.py src/blueprints/api.py src/blueprints/matching_bp.py src/blueprints/helpers.py
  • docker compose -f docker-compose.dev.yml build pagekeeper-dev
  • docker compose -f docker-compose.dev.yml up -d pagekeeper-dev
  • curl http://localhost:4478/

Note

Replace suggestions system with detected books across API, dashboard, and database

  • Replaces the PendingSuggestion-based suggestions system with a new DetectedBook model, adding a detected_books table via Alembic migration and a DetectedRepository for CRUD operations including upsert with status preservation.
  • Replaces /api/suggestions/* endpoints with /api/detected, /api/detected/<source_id>/dismiss, and /api/detected/<source_id>/resolve in api.py.
  • Updates SuggestionService to upsert DetectedBook records instead of PendingSuggestion entries; detected-book creation now runs regardless of SUGGESTIONS_ENABLED unless the entry is dismissed.
  • Updates the dashboard and index template to show a 'Detected' section (driven by detected_books) with source-specific match/add actions, replacing the 'New Suggestions' banner.
  • Epub cache pruning in SyncManager now uses get_all_ebook_filenames() from detected books instead of iterating actionable suggestion matches.
  • Risk: All /api/suggestions/* endpoints, the /suggestions page, and the settings controls for suggestions are removed with no backward compatibility.
📊 Macroscope summarized 9291c26. 23 files reviewed, 4 issues evaluated, 1 issue filtered, 3 comments posted

🗂️ Filtered Issues

src/blueprints/dashboard.py — 0 comments posted, 1 evaluated, 1 filtered
  • line 306: Missing null-safety check on bl_meta.raw_metadata_dict. If bl_meta is not None but bl_meta.raw_metadata_dict is None, calling .get("id") on None will raise AttributeError. The same pattern is handled correctly in resolve_book_covers using (grimmory_meta.raw_metadata_dict or {}).get("id"). [ Out of scope ]

Summary by CodeRabbit

Release Notes

  • New Features

    • Added "Detected Books" dashboard widget to discover, track, and manage content from multiple sources with progress monitoring and status indicators.
  • Refactor

    • Replaced suggestions workflow with integrated detected books system for better content discovery and management.
    • Optimized internal Git utilities.
  • Chores

    • Updated project dependencies.

serabi added 11 commits April 5, 2026 20:16
Track ABS and KoSync activity even when no pairing candidates\nexist so the dashboard can move to a true detected-book queue.\nKeep legacy suggestions intact while adding schema, repository,\nand backend detection support.
…o DetectedBook

- Set SUGGESTIONS_ENABLED default to 'false' to disable legacy rescan
- Add get_all_ebook_filenames() to DetectedRepository
- Update sync_manager cache cleanup to use DetectedBook instead of PendingSuggestion
- PendingSuggestion table remains for upgrade compatibility
- Remove /api/suggestions/* endpoints from api.py (7 routes removed)
- Remove dismissSuggestion from dashboard.js
- Remove clearStaleSuggestions from settings.js
- Remove Clear Stale Suggestions button from settings.html
- Keep serialize_suggestion import for matching_bp.py and dashboard.py (still used)
- Keep suggestions.html, suggestions.js, suggestions.css as orphaned files (can delete later)

Upgrade safe: PendingSuggestion table remains in database.
Delete suggestions.html, suggestions.js, suggestions.css - no longer referenced
Keeps settings.html toggle for SUGGESTIONS_ENABLED env var (still used for rescan)
- Remove legacy suggestion banner from dashboard.py
- Remove suggestions toggle from settings.html
- Remove serialize_suggestion and _has_bookfusion_evidence from helpers.py
- Remove unused imports from api.py, dashboard.py, matching_bp.py
- Add missing imports to dashboard.py (get_container, get_abs_service, etc.)
- Add serialize_detected_book function to helpers.py
- Import get_service_web_url and get_hardcover_book_url from service_url_helper
Stop creating PendingSuggestion rows for ABS detection and keep ABS
progress discovery on the DetectedBook path only.

Also pin mistune to 3.2.0 so the dev image can build again.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 6, 2026

Warning

Rate limit exceeded

@serabi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 50 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 50 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 10eedd4c-56b9-4e9b-b195-eefe93960f9d

📥 Commits

Reviewing files that changed from the base of the PR and between df353f2 and db1816e.

📒 Files selected for processing (7)
  • alembic/versions/r5s6t7u8v9w0_remove_bookfusion_matched_abs_id.py
  • alembic/versions/t9u0v1w2x3y4_merge_detected_books_head.py
  • src/blueprints/api.py
  • src/blueprints/dashboard.py
  • src/db/database_service.py
  • src/sync_manager.py
  • tests/test_database_service_integration.py
📝 Walkthrough

Walkthrough

This PR replaces the suggestions system with a detected books system across the entire codebase. It adds database schema and models for tracking detected books, removes all suggestions-related API endpoints and UI, introduces detected books API endpoints and dashboard display, and updates services to persist detected book state and resolve them when books are mapped.

Changes

Cohort / File(s) Summary
Git hooks & scripts
.githooks/pre-push, scripts/git/create-draft-branch.sh, scripts/git/promote.sh, scripts/git/setup-worktrees.sh, scripts/git/sync-private-mirrors.sh
Removed branch-gating logic in pre-push; deleted create-draft-branch and sync-private-mirrors scripts; updated promote.sh example; switched setup-worktrees.sh from draft to dev branch tracking.
Database schema & models
alembic/versions/s1t2u3v4w5x6_add_detected_books_table.py, alembic/versions/t9u0v1w2x3y4_merge_detected_books_head.py, src/db/models.py
Added detected_books table with migration, merge migration head, and DetectedBook ORM model with uniqueness constraint on (source_id, source), metadata fields, timestamps, and a matches property.
Data access layer
src/db/detected_repository.py, src/db/database_service.py
Introduced DetectedRepository with methods for fetching, listing, upserting, dismissing, and resolving detected books; exposed through DatabaseService auto-delegation.
API endpoints
src/blueprints/api.py
Replaced entire /api/suggestions/* endpoint set with /api/detected/* endpoints: GET /api/detected (list active books), POST /api/detected/<source_id>/dismiss, POST /api/detected/<source_id>/resolve; added error handling with 500 fallback.
Dashboard & routes
src/blueprints/dashboard.py, src/blueprints/matching_bp.py, templates/index.html, templates/partials/navbar.html
Replaced suggestions banner with detected books section on dashboard; removed /suggestions route; removed navbar suggestions link; updated template data model from top_suggestions to detected_books with new card layout.
Service layer
src/blueprints/helpers.py, src/services/suggestion_service.py, src/services/kosync_service.py, src/sync_manager.py
Replaced serialize_suggestion with serialize_detected_book; refactored SuggestionService to upsert DetectedBook instead of PendingSuggestion, removed SUGGESTIONS_ENABLED gating; added resolve_detected_book calls in KosyncService create/discovery flows and in matching_bp; updated cache cleanup to source filenames from detected books.
Frontend UI
static/css/kosync.css, static/css/suggestions.css, static/js/dashboard.js, static/js/settings.js, static/js/suggestions.js, templates/suggestions.html, templates/settings.html
Added detected-card CSS with grid/progress/device styling; removed entire suggestions page template, CSS, and JavaScript; replaced suggestion-banner dismissal with detected-card dismissal in dashboard.js; removed clearStaleSuggestions tool action.
Dependencies & tests
requirements.txt, tests/test_database_service_integration.py, tests/test_queue_suggestion.py, tests/test_suggestion_logic.py
Downgraded mistune 3.2.2 → 3.2.0; added integration tests for save_detected_book upsert and resolve_detected_book scope; updated mocks to stub get_detected_book and assert save_detected_book calls.

Possibly related PRs

  • [Refactor] Extract matching routes into matching_bp #34: Overlaps directly in src/blueprints/matching_bp.py — both modify matching/suggestions logic and route structure, with this PR removing suggestions routes and adding detected-book resolve calls.
  • KoSync system overhaul #52: Overlaps directly in src/services/kosync_service.py — both modify KoSync auto-discovery and ebook creation flows; this PR adds detected-book upserts and resolves into those same paths.
  • Release 0.1.8 #36: Overlaps in suggestion endpoint changes — this PR replaces the suggestion API surface that PR #36 touches.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes


Suggestions fade to history, detected books take the stage,
A system reborn, data flows with grace,
From dismissed to resolved, status flows true,
The library awakens—🌟 detected dreams anew!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 56.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: replacing the suggestions system with detected books, which is the primary objective of this multi-file refactor.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-suggestion-revamp

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment on lines +43 to +44
if existing.status == "dismissed" and detected_book.status == "detected":
detected_book.status = "dismissed"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High db/detected_repository.py:43

The docstring promises "preserving dismissed status" but the implementation only preserves it when the incoming book has status == "detected". If a dismissed book is updated with status == "resolved", line 60 overwrites existing.status and the dismissal is lost. Consider removing the and detected_book.status == "detected" condition on line 43 so dismissed status is preserved regardless of incoming status.

-                if existing.status == "dismissed" and detected_book.status == "detected":
+                if existing.status == "dismissed":
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/db/detected_repository.py around lines 43-44:

The docstring promises "preserving dismissed status" but the implementation only preserves it when the incoming book has `status == "detected"`. If a dismissed book is updated with `status == "resolved"`, line 60 overwrites `existing.status` and the dismissal is lost. Consider removing the `and detected_book.status == "detected"` condition on line 43 so dismissed status is preserved regardless of incoming status.

Evidence trail:
src/db/detected_repository.py lines 33-60 at REVIEWED_COMMIT:
- Line 34: docstring "Upsert a detected book while preserving dismissed status."
- Lines 43-44: conditional preservation `if existing.status == "dismissed" and detected_book.status == "detected": detected_book.status = "dismissed"`
- Line 60: unconditional overwrite `existing.status = detected_book.status`

Comment thread static/js/dashboard.js
/* Suggestion banner dismiss */
function dismissSuggestion(sourceId, source, btn) {
/* Detected card dismiss */
function dismissDetected(sourceId, source, btn) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High js/dashboard.js:857

Line 862 concatenates sourceId directly into the selector string without escaping, so sourceId values containing CSS special characters like ", \, or ] cause the selector to be malformed and return null instead of finding the card. The rest of this codebase uses CSS.escape() for this (lines 243, 344, 687). Consider escaping sourceId with CSS.escape() before concatenation.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file static/js/dashboard.js around line 857:

Line 862 concatenates `sourceId` directly into the selector string without escaping, so `sourceId` values containing CSS special characters like `"`, `\`, or `]` cause the selector to be malformed and return `null` instead of finding the card. The rest of this codebase uses `CSS.escape()` for this (lines 243, 344, 687). Consider escaping `sourceId` with `CSS.escape()` before concatenation.

Evidence trail:
static/js/dashboard.js lines 855-870 (line 862 shows unescaped sourceId in selector), lines 240-250 (line 243 shows CSS.escape usage), lines 341-350 (line 344 shows CSS.escape usage), lines 684-692 (line 687 shows CSS.escape usage) at REVIEWED_COMMIT in https://github.com/serabi/pagekeeper

Comment thread tests/test_database_service_integration.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low

def test_create_book(self):

test_create_book now only asserts against the return value of save_book, so it passes even if save_book returns the input object without persisting to the database. The lines that retrieved the record from the database and verified it exists were removed. Consider restoring get_book_by_abs_id verification to ensure the test actually checks persistence.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file tests/test_database_service_integration.py around line 63:

`test_create_book` now only asserts against the return value of `save_book`, so it passes even if `save_book` returns the input object without persisting to the database. The lines that retrieved the record from the database and verified it exists were removed. Consider restoring `get_book_by_abs_id` verification to ensure the test actually checks persistence.

Evidence trail:
tests/test_database_service_integration.py lines 63-79 (REVIEWED_COMMIT) shows current test only asserting against save_book return value. git_diff MERGE_BASE..REVIEWED_COMMIT shows removal of lines 82-85 which called get_book_by_abs_id(test_abs_id) and asserted the retrieved book was not None and had correct abs_id.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
templates/index.html (1)

104-142: ⚠️ Potential issue | 🟠 Major

Render detected books even when there are no mapped books.

This new block is still inside the outer {% if mappings %} gate at Line 38, so a user with only detected ABS/KoSync activity falls straight into the empty state and never sees these cards.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@templates/index.html` around lines 104 - 142, The detected-books block
guarded by "{% if detected_books %} ... {% endif %}" is currently nested inside
the outer "{% if mappings %}" condition so users with only detected ABS/KoSync
activity never see it; move the entire detected-books block (the "{% if
detected_books %}" through its matching "{% endif %}") out of the outer "{% if
mappings %}" scope so it renders independently, ensuring the template still
checks detected_books but is not conditioned on "mappings" being truthy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@alembic/versions/t9u0v1w2x3y4_merge_detected_books_head.py`:
- Line 8: The import line "from typing import Sequence" is mis-ordered per
ruff/isort; update the imports in the migration file so imports are grouped and
alphabetized according to project rules (standard library, third-party, local)
and place "from typing import Sequence" into the correct group, or run
isort/ruff --fix on the file to automatically reorder and format the import
block; ensure the symbol Sequence remains imported and tests/CI pass.

In `@src/blueprints/dashboard.py`:
- Around line 11-20: The import block in src.blueprints.dashboard.py includes
unused symbols causing a Ruff lint failure; remove get_book_or_404,
get_grimmory_client, and serialize_detected_book from the import list (leave
find_grimmory_metadata, get_abs_service, get_container, get_database_service,
get_enabled_grimmory_server_ids, and get_grimmory_client only if actually used)
so only used helpers are imported; update the from src.blueprints.helpers import
(...) line to no longer reference the three unused names to resolve the lint
error.
- Around line 395-417: The current block swallows exceptions from
database_service.get_active_detected_books and duplicates mapping logic; change
it to call database_service.get_active_detected_books(...) inside a try/except
that catches exceptions but logs the error (including exception details) instead
of silently passing, and build detected_books by reusing the existing
serialize_detected_book(d) helper for each item (replace the manual dict
construction with serialize_detected_book(d)); ensure the logged message clearly
identifies the failure (e.g., "failed to load active detected books") and
includes the exception/stack for diagnostics.

In `@src/blueprints/helpers.py`:
- Around line 566-581: The new serialize_detected_book serializer was added but
callers still import/use the old name serialize_suggestion (e.g.,
tests/test_helpers.py and any other modules), causing import failures; either
update all imports and call sites to use serialize_detected_book and its field
names, or add a backwards-compatibility alias in this module (define
serialize_suggestion = serialize_detected_book) and update tests to use the new
name over time—search for serialize_suggestion, serialize_*suggestion*, and any
test helpers and replace them with serialize_detected_book or add the alias to
helpers.py to avoid breakage.

In `@src/db/detected_repository.py`:
- Around line 108-127: get_all_ebook_filenames currently only collects filenames
from DetectedBook.matches entries, so DetectedBook.ebook_filename values (used
by cleanup_cache keep-list) are omitted; update get_all_ebook_filenames to also
add detected.ebook_filename when present (e.g., after computing matches =
detected.matches or [] add a check like if detected.ebook_filename:
filenames.add(detected.ebook_filename)), keeping the existing session.expunge
behavior and returning the combined set so cleanup_cache receives row-level
ebook filenames too.
- Around line 33-68: In save_detected_book: when upserting an existing
DetectedBook, preserve a prior "resolved" status the same way "dismissed" is
preserved—i.e., if existing.status is "resolved" (or in general in
["dismissed","resolved"]) and the incoming detected_book.status is "detected"
then do not overwrite it; set detected_book.status (or keep existing.status) to
the existing value before assigning existing.status = detected_book.status so
resolved entries are not reopened; update the conditional that now checks only
for "dismissed" to include "resolved" and ensure existing.status is ultimately
preserved when appropriate.

In `@src/services/kosync_service.py`:
- Around line 626-639: The current guard around percentage (0.01 <= percentage
<= 0.70) prevents both queuing new suggestions and updating existing detected
progress; move the detected-book update outside that guard so progress is
refreshed for later PUTs. Specifically, keep calling
self._container.suggestion_service().queue_kosync_suggestion(...) only when
percentage is within the 0.01–0.70 range, but always call
self._db.get_detected_book(doc_hash, source="kosync") and, if present, set
detected.progress_percentage = float(percentage) and
self._db.save_detected_book(detected) regardless of the threshold; preserve the
try/except around the suggestion call or around both operations as appropriate
and reference suggestion_service(), queue_kosync_suggestion(),
get_detected_book(), save_detected_book(), and detected.progress_percentage to
locate the code.

In `@src/services/suggestion_service.py`:
- Around line 493-503: The call to self._upsert_detected_book always sets
progress_percentage=0.0 which wipes out real KoSync progress on refresh;
instead, fetch the current stored progress for this source/source_id (e.g., via
the existing get/fetch method for detected books) and pass that existing value
into _upsert_detected_book, or pass None and update
_upsert_detected_book/save_detected_book to only overwrite progress when a
non-None value is provided; update the call site (where _upsert_detected_book is
invoked) to stop hardcoding 0.0 and ensure
_upsert_detected_book/save_detected_book respect a None/no-op value so refreshes
do not reset progress.

In `@src/sync_manager.py`:
- Around line 241-242: cleanup_cache currently only protects filenames from
auto-discovery matches (via database_service.get_all_ebook_filenames()), but
detected-book rows that only set ebook_filename are omitted and may be deleted;
update cleanup_cache() to also fetch and include detected-only ebook filenames
by calling the database_service method that returns ebook_filename values for
detected rows (e.g., database_service.get_all_detected_ebook_filenames() or add
such a helper) and add those filenames to valid_filenames before deleting cached
files so detected EPUBs are preserved.

In `@static/js/dashboard.js`:
- Around line 859-863: The DOM removal only matches by data-source-id and can
remove the wrong card when different providers share the same sourceId; update
the selector used in the fetch success handler to match both attributes
(data-source-id and data-source) so it uniquely targets the DetectedBook tuple
(sourceId, source). In the fetch .then callback where you build var card =
document.querySelector('.detected-card[data-source-id="' + sourceId + '"]'),
change that selector to also include the data-source attribute (using the same
source variable or default 'abs' as used in the request) so it selects
'.detected-card[data-source-id="..."][data-source="..."]' before removing the
element.

In `@templates/index.html`:
- Around line 127-131: The template renders an "Add as Ebook" link even when
d.ebook_filename is empty, producing a dead
/match?ebook_filename=&action=ebook_only link; update the kosync branch of the
template to conditionally render the second anchor only when d.ebook_filename is
truthy (non-empty) by checking d.ebook_filename before emitting the <a> tag so
the "Add as Ebook" button is hidden when no ebook filename exists.

In `@tests/test_database_service_integration.py`:
- Around line 82-127: The new assertions referencing test_abs_id were
accidentally left inside test_resolve_detected_book_scoped_by_source where
test_abs_id is not defined; move the retrieval/assertion lines that use
test_abs_id back into the original test_create_book or alternatively define and
set test_abs_id earlier in test_resolve_detected_book_scoped_by_source so the
variable exists; locate uses of test_abs_id in the test file (look for the
symbol test_abs_id and the functions test_resolve_detected_book_scoped_by_source
and test_create_book) and either relocate the assertions to test_create_book or
initialize test_abs_id appropriately to restore correct test scope and coverage.

In `@tests/test_suggestion_logic.py`:
- Around line 89-90: Update the test to remove the legacy PendingSuggestion DB
write assertion: delete or stop asserting
mock_db.save_pending_suggestion.assert_called_once(), and retain/ensure the
detected-book expectation by keeping
mock_db.save_detected_book.assert_called_once() (or equivalent assert that
save_detected_book was called exactly once); this aligns the test with the PR
that removed the pending-suggestion write path.

---

Outside diff comments:
In `@templates/index.html`:
- Around line 104-142: The detected-books block guarded by "{% if detected_books
%} ... {% endif %}" is currently nested inside the outer "{% if mappings %}"
condition so users with only detected ABS/KoSync activity never see it; move the
entire detected-books block (the "{% if detected_books %}" through its matching
"{% endif %}") out of the outer "{% if mappings %}" scope so it renders
independently, ensuring the template still checks detected_books but is not
conditioned on "mappings" being truthy.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 93f656c3-b54f-433d-b7b6-4a1bd70df3ef

📥 Commits

Reviewing files that changed from the base of the PR and between 63ac1ba and df353f2.

⛔ Files ignored due to path filters (1)
  • scripts/git/README.md is excluded by !**/*.md
📒 Files selected for processing (30)
  • .githooks/pre-push
  • alembic/versions/s1t2u3v4w5x6_add_detected_books_table.py
  • alembic/versions/t9u0v1w2x3y4_merge_detected_books_head.py
  • requirements.txt
  • scripts/git/create-draft-branch.sh
  • scripts/git/promote.sh
  • scripts/git/setup-worktrees.sh
  • scripts/git/sync-private-mirrors.sh
  • src/blueprints/api.py
  • src/blueprints/dashboard.py
  • src/blueprints/helpers.py
  • src/blueprints/matching_bp.py
  • src/db/database_service.py
  • src/db/detected_repository.py
  • src/db/models.py
  • src/services/kosync_service.py
  • src/services/suggestion_service.py
  • src/sync_manager.py
  • static/css/kosync.css
  • static/css/suggestions.css
  • static/js/dashboard.js
  • static/js/settings.js
  • static/js/suggestions.js
  • templates/index.html
  • templates/partials/navbar.html
  • templates/settings.html
  • templates/suggestions.html
  • tests/test_database_service_integration.py
  • tests/test_queue_suggestion.py
  • tests/test_suggestion_logic.py
💤 Files with no reviewable changes (9)
  • templates/partials/navbar.html
  • .githooks/pre-push
  • static/js/settings.js
  • templates/suggestions.html
  • templates/settings.html
  • static/css/suggestions.css
  • scripts/git/create-draft-branch.sh
  • static/js/suggestions.js
  • scripts/git/sync-private-mirrors.sh

Comment thread alembic/versions/t9u0v1w2x3y4_merge_detected_books_head.py Outdated
Comment thread src/blueprints/dashboard.py
Comment on lines +395 to +417
# Active detected books — for dashboard detected section
detected_books = []
try:
active_detected = database_service.get_active_detected_books(limit=10)
for d in active_detected:
detected_books.append(
{
"id": d.id,
"source": d.source,
"source_id": d.source_id,
"title": d.title,
"author": d.author,
"cover_url": d.cover_url,
"progress_percentage": d.progress_percentage,
"first_detected_at": d.first_detected_at.isoformat() if d.first_detected_at else None,
"last_seen_at": d.last_seen_at.isoformat() if d.last_seen_at else None,
"matches": d.matches,
"device": d.device,
"ebook_filename": d.ebook_filename,
}
)
except Exception:
pass
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don’t swallow detected-book load failures.

If get_active_detected_books() starts failing during rollout, the dashboard quietly degrades to an empty list and gives you nothing in logs to diagnose it. Reusing serialize_detected_book() here also removes the duplicated mapping code.

Suggested change
     try:
         active_detected = database_service.get_active_detected_books(limit=10)
-        for d in active_detected:
-            detected_books.append(
-                {
-                    "id": d.id,
-                    "source": d.source,
-                    "source_id": d.source_id,
-                    "title": d.title,
-                    "author": d.author,
-                    "cover_url": d.cover_url,
-                    "progress_percentage": d.progress_percentage,
-                    "first_detected_at": d.first_detected_at.isoformat() if d.first_detected_at else None,
-                    "last_seen_at": d.last_seen_at.isoformat() if d.last_seen_at else None,
-                    "matches": d.matches,
-                    "device": d.device,
-                    "ebook_filename": d.ebook_filename,
-                }
-            )
-    except Exception:
-        pass
+        detected_books = [serialize_detected_book(d) for d in active_detected]
+    except Exception:
+        logger.exception("Failed to load detected books for dashboard")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/blueprints/dashboard.py` around lines 395 - 417, The current block
swallows exceptions from database_service.get_active_detected_books and
duplicates mapping logic; change it to call
database_service.get_active_detected_books(...) inside a try/except that catches
exceptions but logs the error (including exception details) instead of silently
passing, and build detected_books by reusing the existing
serialize_detected_book(d) helper for each item (replace the manual dict
construction with serialize_detected_book(d)); ensure the logged message clearly
identifies the failure (e.g., "failed to load active detected books") and
includes the exception/stack for diagnostics.

Comment thread src/blueprints/helpers.py
Comment on lines +566 to +581
def serialize_detected_book(d):
"""Serialize DetectedBook for template context."""
return {
"id": d.id,
"source": d.source,
"source_id": d.source_id,
"title": d.title,
"author": d.author,
"cover_url": d.cover_url,
"progress_percentage": d.progress_percentage,
"first_detected_at": d.first_detected_at.isoformat() if d.first_detected_at else None,
"last_seen_at": d.last_seen_at.isoformat() if d.last_seen_at else None,
"matches": d.matches,
"device": d.device,
"ebook_filename": d.ebook_filename,
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Detected serializer migration is incomplete across helper consumers

serialize_detected_book is added, but existing imports/usages of removed suggestion helpers (e.g., tests/test_helpers.py importing serialize_suggestion) must be migrated in the same PR or CI will fail on import/use paths.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/blueprints/helpers.py` around lines 566 - 581, The new
serialize_detected_book serializer was added but callers still import/use the
old name serialize_suggestion (e.g., tests/test_helpers.py and any other
modules), causing import failures; either update all imports and call sites to
use serialize_detected_book and its field names, or add a
backwards-compatibility alias in this module (define serialize_suggestion =
serialize_detected_book) and update tests to use the new name over time—search
for serialize_suggestion, serialize_*suggestion*, and any test helpers and
replace them with serialize_detected_book or add the alias to helpers.py to
avoid breakage.

Comment on lines +33 to +68
def save_detected_book(self, detected_book):
"""Upsert a detected book while preserving dismissed status."""
filters = [
DetectedBook.source_id == detected_book.source_id,
DetectedBook.source == detected_book.source,
]
with self.get_session() as session:
existing = session.query(DetectedBook).filter(*filters).first()
now = datetime.now(UTC)
if existing:
if existing.status == "dismissed" and detected_book.status == "detected":
detected_book.status = "dismissed"

if detected_book.title:
existing.title = detected_book.title
if detected_book.author:
existing.author = detected_book.author
if detected_book.cover_url:
existing.cover_url = detected_book.cover_url
if detected_book.matches_json is not None:
existing.matches_json = detected_book.matches_json
if detected_book.device:
existing.device = detected_book.device
if detected_book.ebook_filename:
existing.ebook_filename = detected_book.ebook_filename

existing.progress_percentage = detected_book.progress_percentage
existing.status = detected_book.status
existing.last_seen_at = detected_book.last_seen_at or now
if existing.first_detected_at is None:
existing.first_detected_at = detected_book.first_detected_at or now

session.flush()
session.refresh(existing)
session.expunge(existing)
return existing
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

❓ Verification inconclusive

In SQLAlchemy ORM, when a mapped column is defined as Column(String(20), default="detected"), is the Python attribute populated when the model instance is constructed, or only during INSERT/flush?


In SQLAlchemy ORM, when a mapped column is defined as Column(String(20), default="detected") in a standard declarative mapping (not using MappedAsDataclass or native dataclasses), the Python attribute is not populated with the default value when the model instance is constructed. The default is applied only during the INSERT statement (i.e., during flush/commit), when no value is provided for that column.

Citations:


Script executed:

# First, let's see the DetectedBook model definition
find . -name "models.py" -path "*/db/*" | head -5

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

./src/db/models.py

Script executed:

# Find calls to save_detected_book to see how detected_book objects are created
rg "save_detected_book" -A 3 -B 3

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

tests/test_database_service_integration.py-        self.assertEqual(saved_book.title, "Test Book Creation")
tests/test_database_service_integration.py-        self.assertEqual(saved_book.status, "active")
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:    def test_save_detected_book_creates_and_updates(self):
tests/test_database_service_integration.py-        """Detected books upsert by source and source_id."""
tests/test_database_service_integration.py-        from src.db.models import DetectedBook
tests/test_database_service_integration.py-
--
tests/test_database_service_integration.py-            progress_percentage=0.25,
tests/test_database_service_integration.py-            cover_url="/cover/1",
tests/test_database_service_integration.py-        )
tests/test_database_service_integration.py:        saved = self.db_service.save_detected_book(first)
tests/test_database_service_integration.py-        self.assertEqual(saved.title, "Detected Title")
tests/test_database_service_integration.py-        self.assertAlmostEqual(saved.progress_percentage, 0.25)
tests/test_database_service_integration.py-
--
tests/test_database_service_integration.py-            author="Author Two",
tests/test_database_service_integration.py-            progress_percentage=0.55,
tests/test_database_service_integration.py-        )
tests/test_database_service_integration.py:        updated = self.db_service.save_detected_book(second)
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-        self.assertEqual(updated.id, saved.id)
tests/test_database_service_integration.py-        self.assertEqual(updated.title, "Detected Title Updated")
--
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-        abs_detected = DetectedBook(source="abs", source_id="shared-id", title="ABS", progress_percentage=0.2)
tests/test_database_service_integration.py-        kosync_detected = DetectedBook(source="kosync", source_id="shared-id", title="KOSync", progress_percentage=0.3)
tests/test_database_service_integration.py:        self.db_service.save_detected_book(abs_detected)
tests/test_database_service_integration.py:        self.db_service.save_detected_book(kosync_detected)
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-        self.assertTrue(self.db_service.resolve_detected_book("shared-id", source="abs"))
tests/test_database_service_integration.py-
--
tests/test_suggestion_logic.py-
tests/test_suggestion_logic.py-        # Assert
tests/test_suggestion_logic.py-        self.mock_db.save_pending_suggestion.assert_called_once()
tests/test_suggestion_logic.py:        self.mock_db.save_detected_book.assert_called_once()
tests/test_suggestion_logic.py-
tests/test_suggestion_logic.py-    def test_suggestion_ignored_when_hidden(self):
tests/test_suggestion_logic.py-        """Test that suggestions are NOT created if they were previously hidden."""
--
tests/test_queue_suggestion.py-        }
tests/test_queue_suggestion.py-        self.manager.queue_suggestion("book-789")
tests/test_queue_suggestion.py-        self.mock_abs.get_item_details.assert_called_once_with("book-789")
tests/test_queue_suggestion.py:        self.mock_db.save_detected_book.assert_called_once()
tests/test_queue_suggestion.py-
tests/test_queue_suggestion.py-    def test_thread_safety_prevents_duplicate(self):
tests/test_queue_suggestion.py-        """Second concurrent call for same ID should be skipped."""
--
src/db/detected_repository.py-                session.expunge(item)
src/db/detected_repository.py-            return items
src/db/detected_repository.py-
src/db/detected_repository.py:    def save_detected_book(self, detected_book):
src/db/detected_repository.py-        """Upsert a detected book while preserving dismissed status."""
src/db/detected_repository.py-        filters = [
src/db/detected_repository.py-            DetectedBook.source_id == detected_book.source_id,
--
src/services/kosync_service.py-                detected = self._db.get_detected_book(doc_hash, source="kosync")
src/services/kosync_service.py-                if detected:
src/services/kosync_service.py-                    detected.progress_percentage = float(percentage)
src/services/kosync_service.py:                    self._db.save_detected_book(detected)
src/services/kosync_service.py-            except Exception as e:
src/services/kosync_service.py-                logger.debug(f"KOSync detected-book update failed for {doc_hash[:8]}...: {e}")
src/services/kosync_service.py-
--
src/services/suggestion_service.py-            device=device,
src/services/suggestion_service.py-            ebook_filename=ebook_filename,
src/services/suggestion_service.py-        )
src/services/suggestion_service.py:        return self.database_service.save_detected_book(detected)
src/services/suggestion_service.py-
src/services/suggestion_service.py-    def _get_bookfusion_context(self) -> dict:
src/services/suggestion_service.py-        try:

Script executed:

# Look at the DetectedBook model definition
fd "models.py" -path "*/db/*" -exec cat {} \;

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 2

stderr:

error: 'h' isn't a valid value for '--type <filetype>'
  [possible values: file, directory, symlink, executable, empty, socket, pipe]

For more information try '--help'

Script executed:

# Search for DetectedBook instantiation patterns
rg "DetectedBook\(" -A 2 -B 2

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

tests/test_database_service_integration.py-        from src.db.models import DetectedBook
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:        first = DetectedBook(
tests/test_database_service_integration.py-            source="abs",
tests/test_database_service_integration.py-            source_id="detected-1",
--
tests/test_database_service_integration.py-        self.assertAlmostEqual(saved.progress_percentage, 0.25)
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:        second = DetectedBook(
tests/test_database_service_integration.py-            source="abs",
tests/test_database_service_integration.py-            source_id="detected-1",
--
tests/test_database_service_integration.py-        from src.db.models import DetectedBook
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:        abs_detected = DetectedBook(source="abs", source_id="shared-id", title="ABS", progress_percentage=0.2)
tests/test_database_service_integration.py:        kosync_detected = DetectedBook(source="kosync", source_id="shared-id", title="KOSync", progress_percentage=0.3)
tests/test_database_service_integration.py-        self.db_service.save_detected_book(abs_detected)
tests/test_database_service_integration.py-        self.db_service.save_detected_book(kosync_detected)
--
src/db/models.py-
src/db/models.py-
src/db/models.py:class DetectedBook(Base):
src/db/models.py-    """Model for external books with real progress that are not yet in PageKeeper."""
src/db/models.py-
--
src/db/models.py-    def __repr__(self):
src/db/models.py-        return (
src/db/models.py:            f"<DetectedBook(id={self.id}, source='{self.source}', source_id='{self.source_id}', "
src/db/models.py-            f"status='{self.status}')>"
src/db/models.py-        )
--
src/services/suggestion_service.py-        ebook_filename: str | None = None,
src/services/suggestion_service.py-    ):
src/services/suggestion_service.py:        detected = DetectedBook(
src/services/suggestion_service.py-            source=source,
src/services/suggestion_service.py-            source_id=source_id,

Script executed:

# Read the DetectedBook model definition
cat src/db/models.py | grep -A 30 "class DetectedBook"

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

class DetectedBook(Base):
    """Model for external books with real progress that are not yet in PageKeeper."""

    __tablename__ = "detected_books"
    __table_args__ = (UniqueConstraint("source_id", "source", name="uq_detected_books_source_id_source"),)

    id = Column(Integer, primary_key=True, autoincrement=True)
    source = Column(String(50), default="abs", nullable=False)
    source_id = Column(String(255), nullable=False)
    title = Column(String(500), nullable=True)
    author = Column(String(500), nullable=True)
    cover_url = Column(String(500), nullable=True)
    progress_percentage = Column(Float, default=0.0, nullable=False)
    first_detected_at = Column(DateTime, default=utc_now)
    last_seen_at = Column(DateTime, default=utc_now, onupdate=utc_now)
    status = Column(String(20), default="detected")
    matches_json = Column(Text, nullable=True)
    device = Column(String(128), nullable=True)
    ebook_filename = Column(String(500), nullable=True)

    def __init__(
        self,
        source_id: str,
        title: str,
        progress_percentage: float,
        author: str = None,
        cover_url: str = None,
        status: str = "detected",
        source: str = "abs",
        matches_json: str = None,
        device: str = None,

Script executed:

# Look at the suggestion_service to see how DetectedBook is created
rg -A 15 "detected = DetectedBook" src/services/suggestion_service.py

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

        detected = DetectedBook(
            source=source,
            source_id=source_id,
            title=title or source_id,
            author=author or "",
            cover_url=cover_url,
            progress_percentage=max(0.0, min(progress_percentage, 1.0)),
            matches_json=json.dumps(matches or []),
            device=device,
            ebook_filename=ebook_filename,
        )
        return self.database_service.save_detected_book(detected)

    def _get_bookfusion_context(self) -> dict:
        try:
            bf_books = list(self.database_service.get_bookfusion_books() or [])

Script executed:

# Check the kosync_service usage more carefully
rg -B 5 -A 5 "get_detected_book.*kosync" src/services/kosync_service.py

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

                suggestion_svc.queue_kosync_suggestion(
                    doc_hash,
                    filename=kosync_doc.filename,
                    device=device,
                )
                detected = self._db.get_detected_book(doc_hash, source="kosync")
                if detected:
                    detected.progress_percentage = float(percentage)
                    self._db.save_detected_book(detected)
            except Exception as e:
                logger.debug(f"KOSync detected-book update failed for {doc_hash[:8]}...: {e}")

Script executed:

# Get the full __init__ method of DetectedBook
sed -n '/class DetectedBook/,/def [^_]/p' src/db/models.py | head -80

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

class DetectedBook(Base):
    """Model for external books with real progress that are not yet in PageKeeper."""

    __tablename__ = "detected_books"
    __table_args__ = (UniqueConstraint("source_id", "source", name="uq_detected_books_source_id_source"),)

    id = Column(Integer, primary_key=True, autoincrement=True)
    source = Column(String(50), default="abs", nullable=False)
    source_id = Column(String(255), nullable=False)
    title = Column(String(500), nullable=True)
    author = Column(String(500), nullable=True)
    cover_url = Column(String(500), nullable=True)
    progress_percentage = Column(Float, default=0.0, nullable=False)
    first_detected_at = Column(DateTime, default=utc_now)
    last_seen_at = Column(DateTime, default=utc_now, onupdate=utc_now)
    status = Column(String(20), default="detected")
    matches_json = Column(Text, nullable=True)
    device = Column(String(128), nullable=True)
    ebook_filename = Column(String(500), nullable=True)

    def __init__(
        self,
        source_id: str,
        title: str,
        progress_percentage: float,
        author: str = None,
        cover_url: str = None,
        status: str = "detected",
        source: str = "abs",
        matches_json: str = None,
        device: str = None,
        ebook_filename: str = None,
    ):
        self.source = source
        self.source_id = source_id
        self.title = title
        self.author = author
        self.cover_url = cover_url
        self.progress_percentage = progress_percentage
        self.status = status
        self.matches_json = matches_json
        self.device = device
        self.ebook_filename = ebook_filename
        self.first_detected_at = utc_now()
        self.last_seen_at = utc_now()

    `@property`
    def matches(self):

Script executed:

# Also check if there are any other places creating DetectedBook without status
rg "DetectedBook(" --no-heading | sort | uniq

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stderr:

regex parse error:
    DetectedBook(
                ^
error: unclosed group

Script executed:

# Search for DetectedBook instantiation patterns without escaping issues
rg "DetectedBook\s*\(" -A 8 | grep -v "^--$"

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

tests/test_database_service_integration.py:        first = DetectedBook(
tests/test_database_service_integration.py-            source="abs",
tests/test_database_service_integration.py-            source_id="detected-1",
tests/test_database_service_integration.py-            title="Detected Title",
tests/test_database_service_integration.py-            author="Author One",
tests/test_database_service_integration.py-            progress_percentage=0.25,
tests/test_database_service_integration.py-            cover_url="/cover/1",
tests/test_database_service_integration.py-        )
tests/test_database_service_integration.py-        saved = self.db_service.save_detected_book(first)
tests/test_database_service_integration.py:        second = DetectedBook(
tests/test_database_service_integration.py-            source="abs",
tests/test_database_service_integration.py-            source_id="detected-1",
tests/test_database_service_integration.py-            title="Detected Title Updated",
tests/test_database_service_integration.py-            author="Author Two",
tests/test_database_service_integration.py-            progress_percentage=0.55,
tests/test_database_service_integration.py-        )
tests/test_database_service_integration.py-        updated = self.db_service.save_detected_book(second)
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:        abs_detected = DetectedBook(source="abs", source_id="shared-id", title="ABS", progress_percentage=0.2)
tests/test_database_service_integration.py:        kosync_detected = DetectedBook(source="kosync", source_id="shared-id", title="KOSync", progress_percentage=0.3)
tests/test_database_service_integration.py-        self.db_service.save_detected_book(abs_detected)
tests/test_database_service_integration.py-        self.db_service.save_detected_book(kosync_detected)
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-        self.assertTrue(self.db_service.resolve_detected_book("shared-id", source="abs"))
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-        resolved = self.db_service.get_detected_book("shared-id", source="abs")
tests/test_database_service_integration.py-        still_active = self.db_service.get_detected_book("shared-id", source="kosync")
tests/test_database_service_integration.py-        self.assertEqual(resolved.status, "resolved")
src/db/models.py:class DetectedBook(Base):
src/db/models.py-    """Model for external books with real progress that are not yet in PageKeeper."""
src/db/models.py-
src/db/models.py-    __tablename__ = "detected_books"
src/db/models.py-    __table_args__ = (UniqueConstraint("source_id", "source", name="uq_detected_books_source_id_source"),)
src/db/models.py-
src/db/models.py-    id = Column(Integer, primary_key=True, autoincrement=True)
src/db/models.py-    source = Column(String(50), default="abs", nullable=False)
src/db/models.py-    source_id = Column(String(255), nullable=False)
src/db/models.py:            f"<DetectedBook(id={self.id}, source='{self.source}', source_id='{self.source_id}', "
src/db/models.py-            f"status='{self.status}')>"
src/db/models.py-        )
src/db/models.py-
src/db/models.py-
src/db/models.py-class Setting(Base):
src/db/models.py-    """
src/db/models.py-    Setting model storing application configuration.
src/db/models.py-    """
src/services/suggestion_service.py:        detected = DetectedBook(
src/services/suggestion_service.py-            source=source,
src/services/suggestion_service.py-            source_id=source_id,
src/services/suggestion_service.py-            title=title or source_id,
src/services/suggestion_service.py-            author=author or "",
src/services/suggestion_service.py-            cover_url=cover_url,
src/services/suggestion_service.py-            progress_percentage=max(0.0, min(progress_percentage, 1.0)),
src/services/suggestion_service.py-            matches_json=json.dumps(matches or []),
src/services/suggestion_service.py-            device=device,

Script executed:

# Check the test cases to understand the actual behavior
cat tests/test_database_service_integration.py | grep -A 20 "test_save_detected_book_creates_and_updates"

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

    def test_save_detected_book_creates_and_updates(self):
        """Detected books upsert by source and source_id."""
        from src.db.models import DetectedBook

        first = DetectedBook(
            source="abs",
            source_id="detected-1",
            title="Detected Title",
            author="Author One",
            progress_percentage=0.25,
            cover_url="/cover/1",
        )
        saved = self.db_service.save_detected_book(first)
        self.assertEqual(saved.title, "Detected Title")
        self.assertAlmostEqual(saved.progress_percentage, 0.25)

        second = DetectedBook(
            source="abs",
            source_id="detected-1",
            title="Detected Title Updated",
            author="Author Two",

Script executed:

# Check if resolve_detected_book exists and what it does
rg "resolve_detected_book|status.*resolved" -A 3 -B 3

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

tests/test_database_service_integration.py-        self.assertEqual(updated.author, "Author Two")
tests/test_database_service_integration.py-        self.assertAlmostEqual(updated.progress_percentage, 0.55)
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:    def test_resolve_detected_book_scoped_by_source(self):
tests/test_database_service_integration.py-        """Resolving a detected book only affects the matching source row."""
tests/test_database_service_integration.py-        from src.db.models import DetectedBook
tests/test_database_service_integration.py-
--
tests/test_database_service_integration.py-        self.db_service.save_detected_book(abs_detected)
tests/test_database_service_integration.py-        self.db_service.save_detected_book(kosync_detected)
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:        self.assertTrue(self.db_service.resolve_detected_book("shared-id", source="abs"))
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-        resolved = self.db_service.get_detected_book("shared-id", source="abs")
tests/test_database_service_integration.py-        still_active = self.db_service.get_detected_book("shared-id", source="kosync")
tests/test_database_service_integration.py:        self.assertEqual(resolved.status, "resolved")
tests/test_database_service_integration.py-        self.assertEqual(still_active.status, "detected")
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-    def test_delete_book(self):
--
src/services/kosync_service.py-                    )
src/services/kosync_service.py-                    self._db.save_book(book, is_new=True)
src/services/kosync_service.py-                    self._db.link_kosync_document(doc_hash, book.id, book.abs_id)
src/services/kosync_service.py:                    self._db.resolve_detected_book(doc_hash, source="kosync")
src/services/kosync_service.py-                    self._db.resolve_suggestion(doc_hash)
src/services/kosync_service.py-                    logger.info(
src/services/kosync_service.py-                        f"Auto-created book '{match['title']}' from exact title match (abs_id={match['abs_id']})"
--
src/services/kosync_service.py-        )
src/services/kosync_service.py-        self._db.save_book(book, is_new=True)
src/services/kosync_service.py-        self._db.link_kosync_document(doc_hash, book.id, book.abs_id)
src/services/kosync_service.py:        self._db.resolve_detected_book(doc_hash, source="kosync")
src/services/kosync_service.py-        self._db.resolve_suggestion(doc_hash)
src/services/kosync_service.py-        logger.info(f"Created ebook-only book: {book.id} '{title}'" + (f" -> {epub_filename}" if epub_filename else ""))
src/services/kosync_service.py-
--
src/blueprints/matching_bp.py-    database_service.resolve_suggestion(abs_id)
src/blueprints/matching_bp.py-    database_service.resolve_suggestion(kosync_doc_id)
src/blueprints/matching_bp.py-    # Also resolve detected entries
src/blueprints/matching_bp.py:    database_service.resolve_detected_book(abs_id, source="abs")
src/blueprints/matching_bp.py-    if kosync_doc_id:
src/blueprints/matching_bp.py:        database_service.resolve_detected_book(kosync_doc_id, source="kosync")
src/blueprints/matching_bp.py-    try:
src/blueprints/matching_bp.py-        device_doc = database_service.get_kosync_doc_by_filename(ebook_filename)
src/blueprints/matching_bp.py-        if device_doc and device_doc.document_hash != kosync_doc_id:
src/blueprints/matching_bp.py-            database_service.resolve_suggestion(device_doc.document_hash)
src/blueprints/matching_bp.py:            database_service.resolve_detected_book(device_doc.document_hash, source="kosync")
src/blueprints/matching_bp.py-    except Exception as e:
src/blueprints/matching_bp.py-        logger.warning(f"Failed to check/resolve device hash: {e}")
src/blueprints/matching_bp.py-
--
src/blueprints/matching_bp.py-            abs_service.add_to_collection(abs_id, current_app.config["ABS_COLLECTION_NAME"])
src/blueprints/matching_bp.py-            attempt_hardcover_automatch(container, book)
src/blueprints/matching_bp.py-            database_service.resolve_suggestion(abs_id)
src/blueprints/matching_bp.py:            database_service.resolve_detected_book(abs_id, source="abs")
src/blueprints/matching_bp.py-            return redirect(url_for("dashboard.index"))
src/blueprints/matching_bp.py-
src/blueprints/matching_bp.py-        # --- Ebook-only import (no audiobook required) ---
--
src/blueprints/matching_bp.py-            # Resolve any suggestions involving these source IDs
src/blueprints/matching_bp.py-            if kosync_doc_id:
src/blueprints/matching_bp.py-                database_service.resolve_suggestion(kosync_doc_id, source="kosync")
src/blueprints/matching_bp.py:                database_service.resolve_detected_book(kosync_doc_id, source="kosync")
src/blueprints/matching_bp.py-            if storyteller_uuid:
src/blueprints/matching_bp.py-                database_service.resolve_suggestion(storyteller_uuid, source="storyteller")
src/blueprints/matching_bp.py:                database_service.resolve_detected_book(storyteller_uuid, source="storyteller")
src/blueprints/matching_bp.py-            if ebook_filename:
src/blueprints/matching_bp.py-                database_service.resolve_suggestion(ebook_filename, source="grimmory")
src/blueprints/matching_bp.py-            return redirect(url_for("dashboard.index"))
--
src/blueprints/matching_bp.py-                except Exception as e:
src/blueprints/matching_bp.py-                    logger.warning(f"Grimmory add_to_shelf failed for '{sanitize_log_data(ebook_filename)}': {e}")
src/blueprints/matching_bp.py-            database_service.resolve_suggestion(kosync_doc_id)
src/blueprints/matching_bp.py:            database_service.resolve_detected_book(kosync_doc_id, source="kosync")
src/blueprints/matching_bp.py-            return redirect(url_for("dashboard.index"))
src/blueprints/matching_bp.py-
src/blueprints/matching_bp.py-        # --- Attach audiobook to ebook-only book ---
--
src/blueprints/matching_bp.py-                raise
src/blueprints/matching_bp.py-            attempt_hardcover_automatch(container, new_book)
src/blueprints/matching_bp.py-            database_service.resolve_suggestion(abs_id)
src/blueprints/matching_bp.py:            database_service.resolve_detected_book(abs_id, source="abs")
src/blueprints/matching_bp.py-            if new_book.kosync_doc_id:
src/blueprints/matching_bp.py-                database_service.resolve_suggestion(new_book.kosync_doc_id)
src/blueprints/matching_bp.py:                database_service.resolve_detected_book(new_book.kosync_doc_id, source="kosync")
src/blueprints/matching_bp.py-            return redirect(url_for("dashboard.index"))
src/blueprints/matching_bp.py-
src/blueprints/matching_bp.py-        # --- Standard flow (requires audiobook) ---
--
src/blueprints/matching_bp.py-                        abs_service.add_to_collection(item["abs_id"], current_app.config["ABS_COLLECTION_NAME"])
src/blueprints/matching_bp.py-                        attempt_hardcover_automatch(container, book)
src/blueprints/matching_bp.py-                        database_service.resolve_suggestion(item["abs_id"])
src/blueprints/matching_bp.py:                        database_service.resolve_detected_book(item["abs_id"], source="abs")
src/blueprints/matching_bp.py-                        continue
src/blueprints/matching_bp.py-
src/blueprints/matching_bp.py-                    # Handle ebook-only queue items
--
src/blueprints/matching_bp.py-                        ensure_kosync_document(book, database_service)
src/blueprints/matching_bp.py-                        if kosync_doc_id:
src/blueprints/matching_bp.py-                            database_service.resolve_suggestion(kosync_doc_id)
src/blueprints/matching_bp.py:                            database_service.resolve_detected_book(kosync_doc_id, source="kosync")
src/blueprints/matching_bp.py-                        continue
src/blueprints/matching_bp.py-
src/blueprints/matching_bp.py-                    book, error = _create_book_mapping(
--
src/blueprints/api.py-
src/blueprints/api.py-
src/blueprints/api.py-@api_bp.route("/api/detected/<source_id>/resolve", methods=["POST"])
src/blueprints/api.py:def resolve_detected_book(source_id):
src/blueprints/api.py-    """Mark a detected book as resolved (added to library)."""
src/blueprints/api.py-    database_service = get_database_service()
src/blueprints/api.py-    source = request.args.get("source", "abs")
src/blueprints/api.py-    if source not in _VALID_SUGGESTION_SOURCES:
src/blueprints/api.py-        return jsonify({"success": False, "error": "Invalid source"}), 400
src/blueprints/api.py:    if database_service.resolve_detected_book(source_id, source=source):
src/blueprints/api.py-        return jsonify({"success": True})
src/blueprints/api.py-    return jsonify({"success": False, "error": "Not found"}), 404
src/blueprints/api.py-
--
src/db/detected_repository.py-            detected.last_seen_at = datetime.now(UTC)
src/db/detected_repository.py-            return True
src/db/detected_repository.py-
src/db/detected_repository.py:    def resolve_detected_book(self, source_id, source="abs"):
src/db/detected_repository.py-        with self.get_session() as session:
src/db/detected_repository.py-            detected = (
src/db/detected_repository.py-                session.query(DetectedBook)
--
src/db/detected_repository.py-            )
src/db/detected_repository.py-            if not detected:
src/db/detected_repository.py-                return False
src/db/detected_repository.py:            detected.status = "resolved"
src/db/detected_repository.py-            detected.last_seen_at = datetime.now(UTC)
src/db/detected_repository.py-            return True
src/db/detected_repository.py-

Script executed:

# Look at the full save_detected_book implementation
cat src/db/detected_repository.py | grep -A 50 "def save_detected_book"

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

    def save_detected_book(self, detected_book):
        """Upsert a detected book while preserving dismissed status."""
        filters = [
            DetectedBook.source_id == detected_book.source_id,
            DetectedBook.source == detected_book.source,
        ]
        with self.get_session() as session:
            existing = session.query(DetectedBook).filter(*filters).first()
            now = datetime.now(UTC)
            if existing:
                if existing.status == "dismissed" and detected_book.status == "detected":
                    detected_book.status = "dismissed"

                if detected_book.title:
                    existing.title = detected_book.title
                if detected_book.author:
                    existing.author = detected_book.author
                if detected_book.cover_url:
                    existing.cover_url = detected_book.cover_url
                if detected_book.matches_json is not None:
                    existing.matches_json = detected_book.matches_json
                if detected_book.device:
                    existing.device = detected_book.device
                if detected_book.ebook_filename:
                    existing.ebook_filename = detected_book.ebook_filename

                existing.progress_percentage = detected_book.progress_percentage
                existing.status = detected_book.status
                existing.last_seen_at = detected_book.last_seen_at or now
                if existing.first_detected_at is None:
                    existing.first_detected_at = detected_book.first_detected_at or now

                session.flush()
                session.refresh(existing)
                session.expunge(existing)
                return existing

            session.add(detected_book)
            session.flush()
            session.refresh(detected_book)
            session.expunge(detected_book)
            return detected_book

    def dismiss_detected_book(self, source_id, source="abs"):
        with self.get_session() as session:
            detected = (
                session.query(DetectedBook)
                .filter(
                    DetectedBook.source_id == source_id,
                    DetectedBook.source == source,
                )

Script executed:

# See if a resolved book can be fetched and updated via kosync
rg "get_detected_book" -B 3 -A 5

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

tests/test_database_service_integration.py-
tests/test_database_service_integration.py-        self.assertTrue(self.db_service.resolve_detected_book("shared-id", source="abs"))
tests/test_database_service_integration.py-
tests/test_database_service_integration.py:        resolved = self.db_service.get_detected_book("shared-id", source="abs")
tests/test_database_service_integration.py:        still_active = self.db_service.get_detected_book("shared-id", source="kosync")
tests/test_database_service_integration.py-        self.assertEqual(resolved.status, "resolved")
tests/test_database_service_integration.py-        self.assertEqual(still_active.status, "detected")
tests/test_database_service_integration.py-
tests/test_database_service_integration.py-    def test_delete_book(self):
tests/test_database_service_integration.py-        """Test deleting a book record with cascading deletes for states and hardcover details."""
--
tests/test_suggestion_logic.py-        self.mock_db.get_all_books.return_value = []
tests/test_suggestion_logic.py-        self.mock_db.get_pending_suggestion.return_value = None
tests/test_suggestion_logic.py-        self.mock_db.suggestion_exists.return_value = False
tests/test_suggestion_logic.py:        self.mock_db.get_detected_book.return_value = None
tests/test_suggestion_logic.py-
tests/test_suggestion_logic.py-        # Prepare successful suggestion creation mocks
tests/test_suggestion_logic.py-        self.mock_abs.get_item_details.return_value = {
tests/test_suggestion_logic.py-            "media": {"metadata": {"title": "Test Book", "authorName": "Author"}}
tests/test_suggestion_logic.py-        }
--
tests/test_queue_suggestion.py-
tests/test_queue_suggestion.py-    def test_creates_suggestion_for_new_book(self):
tests/test_queue_suggestion.py-        self.mock_db.suggestion_exists.return_value = False
tests/test_queue_suggestion.py:        self.mock_db.get_detected_book.return_value = None
tests/test_queue_suggestion.py-        self.mock_abs.get_item_details.return_value = {
tests/test_queue_suggestion.py-            "media": {"metadata": {"title": "Test Book", "authorName": "Author"}}
tests/test_queue_suggestion.py-        }
tests/test_queue_suggestion.py-        self.manager.queue_suggestion("book-789")
tests/test_queue_suggestion.py-        self.mock_abs.get_item_details.assert_called_once_with("book-789")
--
tests/test_queue_suggestion.py-    def test_thread_safety_prevents_duplicate(self):
tests/test_queue_suggestion.py-        """Second concurrent call for same ID should be skipped."""
tests/test_queue_suggestion.py-        self.mock_db.suggestion_exists.return_value = False
tests/test_queue_suggestion.py:        self.mock_db.get_detected_book.return_value = None
tests/test_queue_suggestion.py-
tests/test_queue_suggestion.py-        # Simulate first call in-flight
tests/test_queue_suggestion.py-        self.manager.suggestion_service._suggestion_in_flight.add("book-dup")
tests/test_queue_suggestion.py-        self.manager.queue_suggestion("book-dup")
tests/test_queue_suggestion.py-
--
tests/test_queue_suggestion.py-
tests/test_queue_suggestion.py-    def test_cleans_up_in_flight_on_error(self):
tests/test_queue_suggestion.py-        self.mock_db.suggestion_exists.return_value = False
tests/test_queue_suggestion.py:        self.mock_db.get_detected_book.return_value = None
tests/test_queue_suggestion.py-        self.mock_abs.get_item_details.side_effect = Exception("boom")
tests/test_queue_suggestion.py-
tests/test_queue_suggestion.py-        self.manager.queue_suggestion("book-err")
tests/test_queue_suggestion.py-
tests/test_queue_suggestion.py-        # Should clean up despite error
--
src/blueprints/api.py-
src/blueprints/api.py-
src/blueprints/api.py-@api_bp.route("/api/detected", methods=["GET"])
src/blueprints/api.py:def get_detected_books():
src/blueprints/api.py-    """Return active detected books."""
src/blueprints/api.py-    database_service = get_database_service()
src/blueprints/api.py-    try:
src/blueprints/api.py-        detected = database_service.get_active_detected_books(limit=50)
src/blueprints/api.py-        results = []
--
src/db/detected_repository.py-class DetectedRepository(BaseRepository):
src/db/detected_repository.py-    ACTIVE_STATUSES = ("detected",)
src/db/detected_repository.py-
src/db/detected_repository.py:    def get_detected_book(self, source_id, source="abs"):
src/db/detected_repository.py-        return self._get_one(
src/db/detected_repository.py-            DetectedBook,
src/db/detected_repository.py-            DetectedBook.source_id == source_id,
src/db/detected_repository.py-            DetectedBook.source == source,
src/db/detected_repository.py-        )
--
src/services/suggestion_service.py-
src/services/suggestion_service.py-    def _detected_book_is_dismissed(self, source_id: str, source: str = "abs") -> bool:
src/services/suggestion_service.py-        """Return True when a dismissed detected entry should stay hidden."""
src/services/suggestion_service.py:        detected = self.database_service.get_detected_book(source_id, source=source)
src/services/suggestion_service.py-        return bool(detected and detected.status == "dismissed")
src/services/suggestion_service.py-
src/services/suggestion_service.py-    def _get_storyteller_books_with_progress(self, mapped_uuids: set | None = None) -> list[dict]:
src/services/suggestion_service.py-        """Fetch Storyteller books with 1-70% progress, excluding already-mapped UUIDs."""
src/services/suggestion_service.py-        if not self.storyteller_client or not self.storyteller_client.is_configured():
--
src/services/kosync_service.py-                    filename=kosync_doc.filename,
src/services/kosync_service.py-                    device=device,
src/services/kosync_service.py-                )
src/services/kosync_service.py:                detected = self._db.get_detected_book(doc_hash, source="kosync")
src/services/kosync_service.py-                if detected:
src/services/kosync_service.py-                    detected.progress_percentage = float(percentage)
src/services/kosync_service.py-                    self._db.save_detected_book(detected)
src/services/kosync_service.py-            except Exception as e:
src/services/kosync_service.py-                logger.debug(f"KOSync detected-book update failed for {doc_hash[:8]}...: {e}")

Script executed:

# Check if there's a scenario where kosync updates a resolved book
cat src/services/kosync_service.py | grep -B 10 -A 10 "get_detected_book"

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

        self._db.save_kosync_document(kosync_doc)

        if 0.01 <= percentage <= 0.70:
            try:
                suggestion_svc = self._container.suggestion_service()
                suggestion_svc.queue_kosync_suggestion(
                    doc_hash,
                    filename=kosync_doc.filename,
                    device=device,
                )
                detected = self._db.get_detected_book(doc_hash, source="kosync")
                if detected:
                    detected.progress_percentage = float(percentage)
                    self._db.save_detected_book(detected)
            except Exception as e:
                logger.debug(f"KOSync detected-book update failed for {doc_hash[:8]}...: {e}")

        # Update linked book if exists
        linked_book = None
        if kosync_doc.linked_book_id:
            linked_book = self._db.get_book_by_id(kosync_doc.linked_book_id)

Script executed:

# Check if there's a scenario where a book moves from resolved back to detected
cat src/db/detected_repository.py | grep -B 5 -A 15 "def get_active_detected_books"

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

            DetectedBook,
            DetectedBook.source_id == source_id,
            DetectedBook.source == source,
        )

    def get_active_detected_books(self, limit=None):
        with self.get_session() as session:
            query = (
                session.query(DetectedBook)
                .filter(DetectedBook.status.in_(self.ACTIVE_STATUSES))
                .order_by(DetectedBook.last_seen_at.desc())
            )
            if limit is not None:
                query = query.limit(limit)
            items = query.all()
            for item in items:
                session.expunge(item)
            return items

    def save_detected_book(self, detected_book):
        """Upsert a detected book while preserving dismissed status."""

Script executed:

# Look at ACTIVE_STATUSES to understand which statuses should be displayed
rg "ACTIVE_STATUSES" -B 2 -A 5

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

src/db/detected_repository.py-
src/db/detected_repository.py-class DetectedRepository(BaseRepository):
src/db/detected_repository.py:    ACTIVE_STATUSES = ("detected",)
src/db/detected_repository.py-
src/db/detected_repository.py-    def get_detected_book(self, source_id, source="abs"):
src/db/detected_repository.py-        return self._get_one(
src/db/detected_repository.py-            DetectedBook,
src/db/detected_repository.py-            DetectedBook.source_id == source_id,
--
src/db/detected_repository.py-            query = (
src/db/detected_repository.py-                session.query(DetectedBook)
src/db/detected_repository.py:                .filter(DetectedBook.status.in_(self.ACTIVE_STATUSES))
src/db/detected_repository.py-                .order_by(DetectedBook.last_seen_at.desc())
src/db/detected_repository.py-            )
src/db/detected_repository.py-            if limit is not None:
src/db/detected_repository.py-                query = query.limit(limit)
src/db/detected_repository.py-            items = query.all()
--
src/db/detected_repository.py-                session.query(DetectedBook)
src/db/detected_repository.py-                .filter(
src/db/detected_repository.py:                    DetectedBook.status.in_(self.ACTIVE_STATUSES),
src/db/detected_repository.py-                    DetectedBook.matches_json.isnot(None),
src/db/detected_repository.py-                )
src/db/detected_repository.py-                .all()
src/db/detected_repository.py-            )
src/db/detected_repository.py-            filenames = set()

Script executed:

# Double-check: are all DetectedBook instantiations either passing status or using the default?
rg "DetectedBook\s*\(" -A 10 | grep -E "(DetectedBook|status=)" | head -50

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

tests/test_database_service_integration.py:        first = DetectedBook(
tests/test_database_service_integration.py:        second = DetectedBook(
tests/test_database_service_integration.py:        abs_detected = DetectedBook(source="abs", source_id="shared-id", title="ABS", progress_percentage=0.2)
tests/test_database_service_integration.py:        kosync_detected = DetectedBook(source="kosync", source_id="shared-id", title="KOSync", progress_percentage=0.3)
src/db/models.py:class DetectedBook(Base):
src/db/models.py:            f"<DetectedBook(id={self.id}, source='{self.source}', source_id='{self.source_id}', "
src/db/models.py-            f"status='{self.status}')>"
src/services/suggestion_service.py:        detected = DetectedBook(

Script executed:

# Final check: verify that all DetectedBook creations in suggestion_service pass a status value or rely on the default
cat src/services/suggestion_service.py | grep -B 2 -A 12 "detected = DetectedBook"

Repository: serabi/pagekeeper


Repository: serabi/pagekeeper
Exit code: 0

stdout:

        ebook_filename: str | None = None,
    ):
        detected = DetectedBook(
            source=source,
            source_id=source_id,
            title=title or source_id,
            author=author or "",
            cover_url=cover_url,
            progress_percentage=max(0.0, min(progress_percentage, 1.0)),
            matches_json=json.dumps(matches or []),
            device=device,
            ebook_filename=ebook_filename,
        )
        return self.database_service.save_detected_book(detected)


Preserve "resolved" status during upsert; currently only "dismissed" is preserved.

When save_detected_book updates an existing row, it unconditionally overwrites status with the incoming value. Since all new DetectedBook instances default to status="detected", this reopens previously resolved entries during progress updates.

For example, when kosync syncs progress on a resolved book, the update resets it to "detected" and it reappears on the dashboard. Preserve "resolved" alongside "dismissed":

🐛 Suggested fix
             existing = session.query(DetectedBook).filter(*filters).first()
             now = datetime.now(UTC)
             if existing:
-                if existing.status == "dismissed" and detected_book.status == "detected":
-                    detected_book.status = "dismissed"
+                if existing.status in {"dismissed", "resolved"} and detected_book.status == "detected":
+                    detected_book.status = existing.status

                 if detected_book.title:
                     existing.title = detected_book.title
@@
-                existing.status = detected_book.status
+                existing.status = detected_book.status if detected_book.status != "detected" else existing.status
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/db/detected_repository.py` around lines 33 - 68, In save_detected_book:
when upserting an existing DetectedBook, preserve a prior "resolved" status the
same way "dismissed" is preserved—i.e., if existing.status is "resolved" (or in
general in ["dismissed","resolved"]) and the incoming detected_book.status is
"detected" then do not overwrite it; set detected_book.status (or keep
existing.status) to the existing value before assigning existing.status =
detected_book.status so resolved entries are not reopened; update the
conditional that now checks only for "dismissed" to include "resolved" and
ensure existing.status is ultimately preserved when appropriate.

Comment thread src/sync_manager.py
Comment on lines +241 to +242
# From Detected Books (covers auto-discovery matches)
valid_filenames.update(self.database_service.get_all_ebook_filenames())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Cache cleanup can drop still-relevant detected EPUBs

Line 242 only protects filenames returned from detected matches. Detected rows that only have ebook_filename (no matches yet) are not protected, so cleanup_cache() can delete files still needed by the detected-books flow.

Proposed hardening
-            # From Detected Books (covers auto-discovery matches)
-            valid_filenames.update(self.database_service.get_all_ebook_filenames())
+            # From Detected Books (covers auto-discovery matches)
+            valid_filenames.update(self.database_service.get_all_ebook_filenames())
+            # Also keep direct detected ebook filenames even when matches are empty
+            for detected in self.database_service.get_active_detected_books():
+                if detected.ebook_filename:
+                    valid_filenames.add(detected.ebook_filename)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sync_manager.py` around lines 241 - 242, cleanup_cache currently only
protects filenames from auto-discovery matches (via
database_service.get_all_ebook_filenames()), but detected-book rows that only
set ebook_filename are omitted and may be deleted; update cleanup_cache() to
also fetch and include detected-only ebook filenames by calling the
database_service method that returns ebook_filename values for detected rows
(e.g., database_service.get_all_detected_ebook_filenames() or add such a helper)
and add those filenames to valid_filenames before deleting cached files so
detected EPUBs are preserved.

Comment thread static/js/dashboard.js
Comment on lines +859 to 863
fetch('/api/detected/' + encodeURIComponent(sourceId) + '/dismiss?source=' + encodeURIComponent(source || 'abs'), { method: 'POST' })
.then(function(r) {
if (!r.ok) throw new Error('Failed to dismiss');
var card = document.getElementById('suggestion-card-' + sourceId);
var card = document.querySelector('.detected-card[data-source-id="' + sourceId + '"]');
if (card) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Key the DOM removal by both source and source_id.

DetectedBook is unique by (source_id, source), but this selector only uses data-source-id. If two providers reuse the same ID, a successful dismiss removes the wrong card from the DOM.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@static/js/dashboard.js` around lines 859 - 863, The DOM removal only matches
by data-source-id and can remove the wrong card when different providers share
the same sourceId; update the selector used in the fetch success handler to
match both attributes (data-source-id and data-source) so it uniquely targets
the DetectedBook tuple (sourceId, source). In the fetch .then callback where you
build var card = document.querySelector('.detected-card[data-source-id="' +
sourceId + '"]'), change that selector to also include the data-source attribute
(using the same source variable or default 'abs' as used in the request) so it
selects '.detected-card[data-source-id="..."][data-source="..."]' before
removing the element.

Comment thread templates/index.html
Comment on lines +127 to +131
{% if d.source == 'abs' %}
<a href="/match?abs_id={{ d.source_id }}&search={{ d.title|urlencode }}" class="btn btn-sm btn-primary">Add to Library</a>
{% elif d.source == 'kosync' %}
<a href="/match?search={{ d.title|urlencode }}" class="btn btn-sm btn-primary">Match</a>
<a href="/match?ebook_filename={{ d.ebook_filename|urlencode if d.ebook_filename }}&action=ebook_only" class="btn btn-sm btn-secondary">Add as Ebook</a>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Hide “Add as Ebook” when no ebook filename is known.

KoSync detections can be created from device-only progress, so d.ebook_filename can legitimately be empty. In that case this renders a dead /match?ebook_filename=&action=ebook_only link.

Suggested change
                         {% elif d.source == 'kosync' %}
                             <a href="/match?search={{ d.title|urlencode }}" class="btn btn-sm btn-primary">Match</a>
-                            <a href="/match?ebook_filename={{ d.ebook_filename|urlencode if d.ebook_filename }}&action=ebook_only" class="btn btn-sm btn-secondary">Add as Ebook</a>
+                            {% if d.ebook_filename %}
+                            <a href="/match?ebook_filename={{ d.ebook_filename|urlencode }}&action=ebook_only" class="btn btn-sm btn-secondary">Add as Ebook</a>
+                            {% endif %}
                         {% else %}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{% if d.source == 'abs' %}
<a href="/match?abs_id={{ d.source_id }}&search={{ d.title|urlencode }}" class="btn btn-sm btn-primary">Add to Library</a>
{% elif d.source == 'kosync' %}
<a href="/match?search={{ d.title|urlencode }}" class="btn btn-sm btn-primary">Match</a>
<a href="/match?ebook_filename={{ d.ebook_filename|urlencode if d.ebook_filename }}&action=ebook_only" class="btn btn-sm btn-secondary">Add as Ebook</a>
{% if d.source == 'abs' %}
<a href="/match?abs_id={{ d.source_id }}&search={{ d.title|urlencode }}" class="btn btn-sm btn-primary">Add to Library</a>
{% elif d.source == 'kosync' %}
<a href="/match?search={{ d.title|urlencode }}" class="btn btn-sm btn-primary">Match</a>
{% if d.ebook_filename %}
<a href="/match?ebook_filename={{ d.ebook_filename|urlencode }}&action=ebook_only" class="btn btn-sm btn-secondary">Add as Ebook</a>
{% endif %}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@templates/index.html` around lines 127 - 131, The template renders an "Add as
Ebook" link even when d.ebook_filename is empty, producing a dead
/match?ebook_filename=&action=ebook_only link; update the kosync branch of the
template to conditionally render the second anchor only when d.ebook_filename is
truthy (non-empty) by checking d.ebook_filename before emitting the <a> tag so
the "Add as Ebook" button is hidden when no ebook filename exists.

Comment thread tests/test_database_service_integration.py
Comment on lines 89 to +90
self.mock_db.save_pending_suggestion.assert_called_once()
self.mock_db.save_detected_book.assert_called_once()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Assertion still expects the removed PendingSuggestion write path

Line 89 enforces a legacy DB write that this PR is removing. Keep the detected-book assertion and drop the pending-suggestion creation expectation.

Suggested test expectation update
-        self.mock_db.save_pending_suggestion.assert_called_once()
+        self.mock_db.save_pending_suggestion.assert_not_called()
         self.mock_db.save_detected_book.assert_called_once()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
self.mock_db.save_pending_suggestion.assert_called_once()
self.mock_db.save_detected_book.assert_called_once()
self.mock_db.save_pending_suggestion.assert_not_called()
self.mock_db.save_detected_book.assert_called_once()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_suggestion_logic.py` around lines 89 - 90, Update the test to
remove the legacy PendingSuggestion DB write assertion: delete or stop asserting
mock_db.save_pending_suggestion.assert_called_once(), and retain/ensure the
detected-book expectation by keeping
mock_db.save_detected_book.assert_called_once() (or equivalent assert that
save_detected_book was called exactly once); this aligns the test with the PR
that removed the pending-suggestion write path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant